zebra: fix SRv6 encap lost during recursive nexthop resolution#21519
zebra: fix SRv6 encap lost during recursive nexthop resolution#21519dawkopagh wants to merge 2 commits intoFRRouting:masterfrom
Conversation
Greptile SummaryThis PR fixes a bug in Confidence Score: 5/5Safe to merge — fix is correct, well-scoped, and covered by a dedicated regression test. The core change is a straightforward two-block addition mirroring the existing MPLS label-stacking pattern. Memory management via XREALLOC is handled correctly (old pointer freed by realloc, all fields explicitly written after resize). The sid_zero guard is consistent with its usage in the existing nexthop block. No regressions are introduced to existing paths since the new newhop block only runs when newhop->nh_srv6 is non-NULL, which was previously silently ignored. A complete regression test suite exercises the exact kernel FIB output. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Z as zebra
participant NHG as nexthop_set_resolved()
participant NH as nexthop_add_srv6_seg6()
Note over Z: Recursive resolution of 198.0.0.150/32<br/>nexthop=static NH (nh_srv6=NULL)<br/>newhop=FIB NH of SRv6 VPN route (nh_srv6=SID)
Z->>NHG: resolve nexthop (newhop, nexthop)
NHG->>NHG: copy MPLS labels (newhop then nexthop)
alt newhop->nh_srv6 != NULL (NEW block)
NHG->>NH: nexthop_add_srv6_seg6(resolved_hop, newhop SID)
NH->>NH: XCALLOC seg6_segs (first call)
NH-->>NHG: resolved_hop->nh_srv6 set with newhop SID
end
alt nexthop->nh_srv6 != NULL (existing block)
NHG->>NH: nexthop_add_srv6_seg6(resolved_hop, nexthop SID)
NH->>NH: XREALLOC if nexthop num_segs > newhop num_segs
NH-->>NHG: parent SID takes precedence
else nexthop->nh_srv6 == NULL (bug fix case)
NHG->>NHG: skip — newhop SID is preserved
end
NHG-->>Z: resolved_hop with correct SRv6 encap
Z->>Z: install route with encap seg6 (not encap mpls)
Reviews (2): Last reviewed commit: "zebra: fix heap overflow and guard incon..." | Re-trigger Greptile |
| if (newhop->nh_srv6->seg6_segs && | ||
| newhop->nh_srv6->seg6_segs->num_segs && | ||
| !sid_zero(newhop->nh_srv6->seg6_segs)) | ||
| nexthop_add_srv6_seg6(resolved_hop, |
There was a problem hiding this comment.
the greptile issue with these apis looks valid to me
When resolving a recursive nexthop, nexthop_set_resolved() copied MPLS labels from both the resolver's FIB nexthop (newhop) and the parent nexthop, but copied SRv6 info only from the parent. As a result, an IPv4 route whose nexthop resolved through an SRv6 VPN route was installed with encap mpls instead of encap seg6, silently breaking traffic forwarding. Fix by adding a newhop->nh_srv6 copy block before the existing nexthop->nh_srv6 block, mirroring the MPLS label stacking logic. Both seg6local action and seg6 SID stack are propagated, a sid_zero() guard prevents copying an uninitialised SID. Signed-off-by: Dawid Kopec <dkopec@akamai.com>
38247a1 to
7212a1e
Compare
Description
When resolving a recursive nexthop, nexthop_set_resolved() copied MPLS labels from both the resolver's FIB nexthop (newhop) and the parent nexthop, but copied SRv6 info only from the parent. As a result, an IPv4 route whose nexthop resolved through an SRv6 VPN route was installed with encap mpls instead of encap seg6, silently breaking traffic forwarding.
Fix
Fixed by adding a newhop->nh_srv6 copy block before the existing nexthop->nh_srv6 block, mirroring the MPLS label stacking logic. Both seg6local action and seg6 SID stack are propagated, a sid_zero() guard prevents copying an uninitialised SID.
Testing
Before fix:
After fix: